Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support of decimal numbers #77

Merged

Conversation

odobosh-lohika-tix
Copy link
Contributor

According to oData documentation decimal number should be represented as [0-9]+.[0-9]+M|m

@odobosh-lohika-tix
Copy link
Contributor Author

Please take a look at these updates

@techniq
Copy link
Owner

techniq commented Jan 11, 2021

@odobosh-lohika-tix Sorry for the delay. It appears all non-integer numbers are being treated as decimals... what about floats?

@odobosh-lohika-tix
Copy link
Contributor Author

@techniq thank you for your notice.
Extended number with double type, added decimal as new type, updated README.
There is a small issue: Double type in js could be represented with 17 precision while 15 in oData specification.

@techniq
Copy link
Owner

techniq commented Jan 12, 2021

@odobosh-lohika-tix Thanks for all the work on this. I see you linked to the OData 2.0 spec. Is this still relevant with OData 4.0? For some reason, I was thinking this was removed (and unneeded). I've used Decimal's in my projects in the past and did not require this change.

@odobosh-lohika-tix
Copy link
Contributor Author

odobosh-lohika-tix commented Jan 12, 2021

@techniq You are right - it was removed in v3 of odata.
But some OData implementations still have some gaps and troubles parsing numeric values. In particular there is a problem in .NET implementation which is by far the most popular(OData/odata.net#461). This little fix will allow the client just explicitly set the type of the value if they want to do so to close the gap, without resorting to raw type.

So no need in double type but we can leave decimal type (as separate one).

@techniq techniq merged commit cc00acd into techniq:master Jan 12, 2021
@techniq
Copy link
Owner

techniq commented Jan 12, 2021

@odobosh-lohika-tix That works for me. Thanks for the PR!

@techniq
Copy link
Owner

techniq commented Jan 12, 2021

@odobosh-lohika-tix Released as 6.7.0. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants